-
Notifications
You must be signed in to change notification settings - Fork 96
feat: Add support for device code grant #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
chalasr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Can you rebase it? Here are some comments also
|
Hi @chalasr I'm not entirely sure I understood correctly what you wanted for the docs/device-code-grant.md improvements, but I tried my best to fit to your feedback :) PR is rebased ! |
|
Interested in this PR, but there's been no activity for a few months. Anything we can do to help? |
|
+1 interested as well ! |
ajgarlag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work here. I've managed to create a working demo. I've reviewed your code and added some suggestions.
| ->scalarNode('device_code_polling_interval') | ||
| ->info('How soon (in seconds) can the device code be used to poll for the access token without being throttled') | ||
| ->defaultValue(5) | ||
| ->end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have an option to enable verification_uri_complete generation, and another one to enable polling interval visibility in the device authorization response.
| ->scalarNode('device_code_polling_interval') | |
| ->info('How soon (in seconds) can the device code be used to poll for the access token without being throttled') | |
| ->defaultValue(5) | |
| ->end() | |
| ->booleanNode('enable_device_code_verification_uri_complete_generation') | |
| ->info('Whether to enable the generation of verification_uri_complete') | |
| ->defaultTrue() | |
| ->end() | |
| ->scalarNode('device_code_polling_interval') | |
| ->info('How soon (in seconds) can the device code be used to poll for the access token without being throttled') | |
| ->defaultValue(5) | |
| ->end() | |
| ->booleanNode('enable_device_code_polling_interval_visibility') | |
| ->info('Whether to enable the visibility of polling interval') | |
| ->defaultTrue() | |
| ->end() |
| /** | ||
| * @param bool $persist Set to true when creating a new device code | ||
| */ | ||
| public function save(DeviceCodeInterface $deviceCode, bool $persist = true): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of the boolean argument, like any other manager.
| /** | |
| * @param bool $persist Set to true when creating a new device code | |
| */ | |
| public function save(DeviceCodeInterface $deviceCode, bool $persist = true): void; | |
| public function save(DeviceCodeInterface $deviceCode): void; |
| } | ||
|
|
||
| private function buildClientEntity(ClientInterface $client): ClientEntity | ||
| public function buildClientEntity(ClientInterface $client): ClientEntity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changed needed?
| $deviceCode = $this->deviceCodeManager->find($deviceCodeEntity->getIdentifier()); | ||
| $newDeviceCode = false; | ||
|
|
||
| if ($deviceCode) { | ||
| if ($deviceCodeEntity->getLastPolledAt()) { | ||
| $deviceCode->setLastPolledAt($deviceCodeEntity->getLastPolledAt()); | ||
| } | ||
| } else { | ||
| $newDeviceCode = true; | ||
| $deviceCode = $this->buildDeviceCodeModel($deviceCodeEntity); | ||
| } | ||
|
|
||
| $this->deviceCodeManager->save($deviceCode, $newDeviceCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need to update the lastPolledAt property. DeviceGrant is already doing it
I think this method should build a new DeviceCode model if not exists (buildDeviceCodeModel), or update it (I would add a updateDeviceCodeModel method) with the values of the received DeviceCodeEntityInterface and finally save it to the DeviceManagerInteface
| ->scalarNode('device_code_verification_uri') | ||
| ->info('The full URI the user will need to visit to enter the user code') | ||
| ->defaultValue('') | ||
| ->end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could use a route name here too. If we detect that the value is not an absolute path nor and absolute URL, we could use the URL generator service to generate the URI.
| $this->deviceCodeManager->save($deviceCode, $newDeviceCode); | ||
| } | ||
|
|
||
| public function approveDeviceCode(string $userCode, string $userId): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether this method should be included in the bundle. I've created a working demo that doesn't use this it.
Either way, I'll move this method to the DeviceCodeManagerInterface and rename it resolveDeviceCode, adding a third required boolean parameter to indicate whether the request has been approved.
This PR adds support for device code grants, which has been available for some time in oauth2-server
Ready to be tested, I'd love to get some feedback if you think some areas could be improved !
Design considerations